Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib.packagesFromDirectoryRecursive: use explicit recursion, support nested scopes #359984

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

nbraud
Copy link
Contributor

@nbraud nbraud commented Nov 28, 2024

Follow-up on #359941

  • Extend lib.filesystem.packagesFromDirectoryRecursive with support for explicit recursion.
  • Add an example showing how to use recurseIntoDirectory.
  • Update packagesFromDirectoryRecursive to create nested scopes for each (sub)directory (not just the top-level one) when newScope is given.
  • Apply recurseIntoAttrs by default when recursing into a directory. (Deferred to follow-up PR)
  • Add a test using scopes for packagesFromDirectoryRecursive, ensuring:
    • packages can depend on other packages in the same scope and in ancestor scopes (the scope's parent or grandparent) ;
    • packages can have mutual dependencies.
  • Reject unknown parameters (breaking change!)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested nix-instantiate --eval --strict lib/tests/misc.nix
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Note
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@nbraud nbraud added 0.kind: enhancement Add something new 9.needs: maintainer feedback 2.status: blocked by pr/issue Another PR or issue is preventing this from being completed needs_reviewer (old Marvin label, do not use) 8.has: tests This PR has tests labels Nov 28, 2024
@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Nov 28, 2024
@nbraud

This comment was marked as resolved.

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 1753d6d to 4c6c2ec Compare November 28, 2024 22:22
@nbraud
Copy link
Contributor Author

nbraud commented Nov 28, 2024

Ran nixfmt over the new test fixtures

lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

Rebased following the parent PR being merged

@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 4c6c2ec to 8a5830c Compare December 1, 2024 10:09
@nbraud nbraud marked this pull request as ready for review December 1, 2024 10:09
@nix-owners nix-owners bot requested a review from infinisil December 1, 2024 10:11
lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud
Copy link
Contributor Author

nbraud commented Dec 1, 2024

I'd prefer to land #359898 first, as there will be a merge conflict over the function's documentation, and having the improved documentation will make documenting this PR's changes simpler.

PS: Marked as draft again, since I'll wait for that to land before writing documentation in this PR.

@nbraud nbraud marked this pull request as draft December 1, 2024 10:45
lib/filesystem.nix Outdated Show resolved Hide resolved
@nbraud nbraud force-pushed the lib/fs/packagesFromDir-nested-scopes branch from 85b9516 to ecca66e Compare December 3, 2024 12:45
@nbraud
Copy link
Contributor Author

nbraud commented Dec 3, 2024

Rebased atop both blocking PRs, added documentation.

Still needs an example how to use recurseIntoDirectory

@nbraud
Copy link
Contributor Author

nbraud commented Dec 30, 2024

This is only waiting for reviews, no functional change since the beginning of the month: only rebased to address the merge conflict and squash fixup commits, and moved the release notes entries. (One further change was tried and reverted, to be done in #369421)

@9999years if you re-review, I think we could get that merged soonish

Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes. Thank you for the efforts that you already put into it.

lib/filesystem.nix Outdated Show resolved Hide resolved
lib/filesystem.nix Outdated Show resolved Hide resolved
nixos/doc/manual/release-notes/rl-2505.section.md Outdated Show resolved Hide resolved
@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 2.status: merge conflict This PR has merge conflicts with the target branch labels Jan 3, 2025
@nbraud
Copy link
Contributor Author

nbraud commented Jan 12, 2025

I'll hold off on rebasing (to address the merge conflict and squash fixup commits) until @hsjobeki can reply, to make the review work easier as it's possible to only display the new changes.

lib/filesystem.nix Outdated Show resolved Hide resolved
This was implicit in the previous “other files are ignored.”
… the lambda

also hoisted the default `recurseIntoDirectory`, as requested during review
@nbraud nbraud requested a review from hsjobeki January 12, 2025 13:03
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 12, 2025
@ofborg ofborg bot removed the ofborg-internal-error Ofborg encountered an error label Jan 12, 2025
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Me and @hsjobeki reviewed this PR more closely together in a call and agreed on this feedback:

  • The new newScope attribute argument and the removal of ... from packagesFromDirectoryRecursive looks good to us.
  • However, the recurseIntoDirectory concept feels a bit messy and hard to understand (we didn't quite get it in our ~1 hour of reviewing this PR together). In particular:
    • It's unclear what processDir does based on the documentation
    • recurseArgs has unclear motivation, is not tested and is missing example documentation.

In alignment with the one PR for each change lib PR guideline, we think this PR should defer the recurseIntoDirectory and recurseArgs introduction to a separate PR that clearly states motivation for the new interfaces and adds missing docs with examples.

Also note that there's now separate Nixpkgs release notes where you need to note the lib changes, which is the cause of the merge conflict.

@nbraud
Copy link
Contributor Author

nbraud commented Jan 18, 2025

Thanks for the review (again!) @hsjobeki & @infinisil !
Sorry for the slow reply, I've been mainly stuck in bed by migraines 😬

[...] the recurseIntoDirectory concept feels a bit messy and hard to understand (we didn't quite get it in our ~1 hour of reviewing this PR together). In particular:

  * It's unclear what `processDir` does based on the documentation
  * `recurseArgs` has unclear motivation, is not tested and is missing example documentation.

Duly noted. FWIW, recurseArgs was introduced when starting to reject unknown args: without a dedicated argument, it becomes very inconvenient for a user-provided recurseIntoDirectory function to keep any data of its own. I just hadn't found a usecase compelling-enough to add an example for, where that's needed.

(I said “very inconvenient” because it's technically doable by changing recurseIntoDirectory between recursive calls; I'm not sure that would even occur as a possibility to most users.)

In alignment with the one PR for each change lib PR guideline, we think this PR should defer the recurseIntoDirectory and recurseArgs introduction to a separate PR that clearly states motivation for the new interfaces and adds missing docs with examples.

I'm somewhat confused there, since adding open recursion is part of how nested scopes are implemented (at least in this PR) but I can remove it from the public API and simplify as much as possible.

Also note that there's now separate Nixpkgs release notes where you need to note the lib changes, which is the cause of the merge conflict.

I'm aware. I simply haven't rebased yet, so it's easier to see which changes are introduced between reviews.

@nbraud nbraud requested a review from infinisil January 18, 2025 20:45
@infinisil
Copy link
Member

I'm somewhat confused there, since adding open recursion is part of how nested scopes are implemented (at least in this PR) but I can remove it from the public API and simplify as much as possible.

The makeScope is already a useful change on its own. The fact that it's more limited allows guaranteeing more about the result in a more abstract sense. I like the saying "Constraints Liberate, Liberties Constrain" here. Of course, it's always possible to remove constraints later (by adding recurseIntoDirectory), but we can't go the other way and add more constraints (that would be a breaking change). So I think it's a good idea to start out with more constraints and loosen them as necessary :)

@nbraud
Copy link
Contributor Author

nbraud commented Jan 18, 2025

The makeScope is already a useful change on its own. [...] So I think it's a good idea to start out with more constraints and loosen them as necessary :)

That's fair. In general I'm not keen on “hard-coding” behaviours like that without any way to pass a different one, but I guess it does cover the majority of usecases.
In any case, the fully-generic implementation will remain in the PR's history anyhow1 if the need occurs.

Footnotes

  1. Assuming GitHub never garbage-collects commits (from the actual git repos) which are linked-to by the PR UI... which I have no idea about 😅
    I guess I ought to tag that locally in case I need it.

nixos/doc/manual/release-notes/rl-2505.section.md Outdated Show resolved Hide resolved
Comment on lines +418 to +425
recurseIntoAttrs (makeScope newScope (self:
# generate the attrset representing the directory, using the new scope's `callPackage` and `newScope`
processDir (args // {
inherit (self) callPackage newScope;
})
))
else
processDir args
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably makes sense to move recurseIntoAttrs into processDir to apply it unconditionally.

Copy link
Contributor Author

@nbraud nbraud Jan 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I initially did so, then pushed it back to a follow-up PR because CI could then find additional derivations (good!) ... some of which were broken.

Thanks for confirming this is the correct thing to do: I thought so as well, but didn't get anyone to pitch in, one way or the other, when I asked for comments on that.

@nbraud nbraud requested a review from infinisil January 19, 2025 09:30
@nbraud nbraud dismissed hsjobeki’s stale review January 19, 2025 09:31

Superseded by joint review with @infinisil

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: enhancement Add something new 2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: lib The Nixpkgs function library 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants